Skip to content

Retry shim start without userns on clone failure#150

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
dmcgowan:retry-on-userns-failure
Apr 14, 2026
Merged

Retry shim start without userns on clone failure#150
dmcgowan merged 1 commit intocontainerd:mainfrom
dmcgowan:retry-on-userns-failure

Conversation

@dmcgowan
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan commented Apr 7, 2026

cloneMntNs sets CLONE_NEWUSER|CLONE_NEWNS on the child, but clone can fail for reasons the proactive AppArmor sysctl check cannot detect — seccomp filters, other LSM policies, or EACCES when inherited socket fds cross the user namespace boundary after exec triggers capability recomputation.

Return whether namespace flags were set so the caller can distinguish a namespace-related Start failure from an unrelated one. On failure, rebuild the command without clone flags and retry, degrading gracefully to no mount isolation rather than failing the container start entirely.

Copilot AI review requested due to automatic review settings April 7, 2026 23:01
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes shim startup more resilient on Linux by retrying Start() without user/mount namespace clone flags when the initial start fails after enabling user namespaces, allowing the shim to degrade to no mount isolation instead of failing the container start.

Changes:

  • Change cloneMntNs to return a boolean indicating whether userns/mntns clone flags were applied.
  • On Unix, retry shim cmd.Start() without namespace isolation when the first start fails and cloneMntNs had enabled user namespaces.
  • Update non-Linux stub implementation to match the new cloneMntNs signature.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
internal/shim/manager/mount_other.go Update non-Linux cloneMntNs stub to return bool (always false).
internal/shim/manager/mount_linux.go Return whether userns/mntns clone flags were applied so callers can distinguish the path taken.
internal/shim/manager/manager_unix.go Retry shim start without namespace isolation when a userns-enabled start fails; add logging for the fallback.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/manager_unix.go Outdated
Comment thread internal/shim/manager/manager_unix.go Outdated
@dmcgowan dmcgowan force-pushed the retry-on-userns-failure branch from ccfb76d to c010af3 Compare April 8, 2026 23:21
Copilot AI review requested due to automatic review settings April 11, 2026 06:28
@dmcgowan dmcgowan force-pushed the retry-on-userns-failure branch from c010af3 to f68d962 Compare April 11, 2026 06:28
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/mount_linux.go Outdated
Comment thread internal/shim/manager/manager_unix.go Outdated
Comment thread internal/shim/manager/manager_unix.go Outdated
@dmcgowan dmcgowan force-pushed the retry-on-userns-failure branch from f68d962 to f040567 Compare April 14, 2026 01:37
cloneMntNs sets CLONE_NEWUSER|CLONE_NEWNS on the child, but clone can
fail for reasons the proactive AppArmor sysctl check cannot detect —
seccomp filters, other LSM policies, or EACCES when inherited socket
fds cross the user namespace boundary after exec triggers capability
recomputation.

Return whether namespace flags were set so the caller can distinguish a
namespace-related Start failure from an unrelated one. On failure, rebuild
the command without clone flags and retry, degrading gracefully to no
mount isolation rather than failing the container start entirely.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Copilot AI review requested due to automatic review settings April 14, 2026 01:48
@dmcgowan dmcgowan force-pushed the retry-on-userns-failure branch from f040567 to 5db9fb1 Compare April 14, 2026 01:48
@dmcgowan dmcgowan marked this pull request as ready for review April 14, 2026 01:48
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/mount_linux.go
Comment thread internal/shim/manager/manager_unix.go
Comment thread internal/shim/manager/manager_unix.go
Comment thread internal/shim/manager/manager_unix.go
@dmcgowan dmcgowan merged commit 01a9334 into containerd:main Apr 14, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants